Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

lib: runtime deprecate process.binding() #48568

Closed
wants to merge 1 commit into from
Closed

lib: runtime deprecate process.binding() #48568

wants to merge 1 commit into from

Conversation

RafaelGSS
Copy link
Member

Following up @jasnell work on runtime deprecation of some modules. This pr deprecates the process.binding entirely, targetting Node.js 21 (non-LTS version).

I'm opening this PR just to see how feasible/impactful is this approach. I wonder if #37485 (review) is still an issue.

cc: @nodejs/tsc

@RafaelGSS RafaelGSS added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 26, 2023
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules
  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Jun 26, 2023
@RafaelGSS RafaelGSS force-pushed the runtime-deprecate-process-binding branch from cc808bc to 3d721f7 Compare June 26, 2023 23:50
@mscdex mscdex added the needs-citgm PRs that need a CITGM CI run. label Jun 27, 2023
bnoordhuis
bnoordhuis reviewed Jun 27, 2023
View reviewed changes
lib/internal/bootstrap/realm.js
Comment on lines -154 to -158
process.emitWarning(
`Access to process.binding('${module}') is deprecated.`,
'DeprecationWarning',
'DEP0111');
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this do the opposite of runtime deprecating?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was emitting a warning whenever a runtime deprecated module is called with process.binding. Example, process.binding('async_wrap'). Now, all the modules are deprecated, so I removed them in favor of:

process.binding = deprecate(process.binding,
                              'process.binding() is deprecated. ' +
    'Please use public APIs instead.', 'DEP0111');

Otherwise, users were going to see two warning deprecations for the same operation.

aduh95
aduh95 reviewed Jun 27, 2023
View reviewed changes
doc/api/deprecations.md Outdated Show resolved Hide resolved
@RafaelGSS
lib: runtime deprecate process.binding
a2b9fec 
Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
@RafaelGSS RafaelGSS force-pushed the runtime-deprecate-process-binding branch from 3d721f7 to a2b9fec Compare June 27, 2023 14:13
@joyeecheung
Copy link
Member

joyeecheung commented Jun 28, 2023 •
edited
Loading

I think a less disruptive approach to deprecate process.binding() could be:

  1. Use the legacyWrapperList mechanism to list the current process.binding() surface explicitly (in particular, stop adding more stuff to the surface, because we don't always pay attention to the additions to these bindings)
  2. Deprecate and remove the problematic properties from the surface, providing alternatives if possible.
  3. Keep surface as harmless as possible, and take things away when we find them problematic

Simply deprecating process.binding() entirely could be quite disruptive - for example, if a dependency of some kind of tool that spins off child processes frequently (e.g. for running tests) uses this API, which is not that rare, a runtime deprecation could result in a lot of noise for the user and forces them to set --no-deprecation to work around it, which in the end could cause more problems. It's also not entirely necessary because even though there are many problematic APIs in process.binding(), there are also a lot of ones that aren't very problematic (read-only or side-effect-free with non-sensitive data). We also haven't really looked into the surface and prepared enough alternatives for them, the "Please use public APIs instead" suggestion isn't really quite useful compared to the per-API suggestions that we theoretically should be able to make.

mcollina
mcollina approved these changes Jun 29, 2023
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@targos
Copy link
Member

targos commented Jun 30, 2023

To assess ecosystem impact, we could change it from warning to throwing an error and run CITGM.

@RafaelGSS
Copy link
Member Author

I think a less disruptive approach to deprecate process.binding() could be:

  1. Use the legacyWrapperList mechanism to list the current process.binding() surface explicitly (in particular, stop adding more stuff to the surface, because we don't always pay attention to the additions to these bindings)
  2. Deprecate and remove the problematic properties from the surface, providing alternatives if possible.
  3. Keep surface as harmless as possible, and take things away when we find them problematic

Simply deprecating process.binding() entirely could be quite disruptive - for example, if a dependency of some kind of tool that spins off child processes frequently (e.g. for running tests) uses this API, which is not that rare, a runtime deprecation could result in a lot of noise for the user and forces them to set --no-deprecation to work around it, which in the end could cause more problems. It's also not entirely necessary because even though there are many problematic APIs in process.binding(), there are also a lot of ones that aren't very problematic (read-only or side-effect-free with non-sensitive data). We also haven't really looked into the surface and prepared enough alternatives for them, the "Please use public APIs instead" suggestion isn't really quite useful compared to the per-API suggestions that we theoretically should be able to make.

So practically speaking, your suggestion is to keep doing what @jasnell was doing on #37576 and subsequent PRs?

My initial thought was to leverage that the next major release is a non-lts version and it's likely the best way to assess the real impact of that feature. If that proves not worth it, we can revert to Node.js 22 -- I know that that's far from ideal and can lead us to some non-great feedback from the community, but I feel that's the only way to really see the impact and plan what to do next.

To assess ecosystem impact, we could change it from warning to throwing an error and run CITGM.

Agree. Regardless if we're going to pursue this work or not, it's good to have a big picture. I'll run it right after fixing the test issues.

@RafaelGSS
Copy link
Member Author

CITGM: https://ci.nodejs.org/job/citgm-smoker/3179/ (rebased on main)

@jasnell
Copy link
Member

jasnell commented Sep 14, 2023

FWIW, I'm fine with the more disruptive approach on this. It's been long enough.

mcollina
mcollina approved these changes Sep 15, 2023
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@aduh95
Copy link
Contributor

aduh95 commented Sep 29, 2023

@RafaelGSS can you please rebase?

@RafaelGSS RafaelGSS mentioned this pull request Nov 15, 2023
Closed
@RafaelGSS
Copy link
Member Author

Closing in favor of #50687

@RafaelGSS RafaelGSS closed this Nov 15, 2023
@RafaelGSS RafaelGSS mentioned this pull request Jul 10, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

@bnoordhuis bnoordhuis bnoordhuis left review comments

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. needs-citgm PRs that need a CITGM CI run. process Issues and PRs related to the process subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@RafaelGSS @nodejs-github-bot @joyeecheung @targos @jasnell @aduh95 @mcollina @bnoordhuis @mscdex